Add map_mut and map_axis_mut by LukeMathWalker · Pull Request #460 · rust-ndarray/ndarray

LukeMathWalker · 2018-05-29T07:12:54Z

Following #452 I tried to implement map_mut and map_axis_mut.

There is unsafe code in multiple spots and my familiarity with the overall architecture of ndarray (and Rust in general) is still lacking, so I'd like to understand if some of my choices might be calling for disaster in disguise:

I have implemented TrustedIterator for IterMut and slide::IterMut - was there any reason this was not already in place (apart from not having been needed so far)?
to avoid issues with the borrow checker in the else branch I have modified

if let Some(slc) = as_slice_memory_order_mut() {
    [...]
} else {
    [...]
}

into

if self.is_contiguous() {
    let slc = self.as_slice_memory_order_mut().unwrap();
    [...]
} else {
    [...]
}

which seemed reasonable looking at as_slice_memory_order_mut implementation:

    /// Return the array’s data as a slice if it is contiguous,
    /// return `None` otherwise.
    pub fn as_slice_memory_order_mut(&mut self) -> Option<&mut [A]>
        where S: DataMut
    {
        if self.is_contiguous() {
            self.ensure_unique();
            unsafe {
                Some(slice::from_raw_parts_mut(self.ptr, self.len()))
            }
        } else {
            None
        }
    }

@jturner314 @rcarson3

…IterMut

bluss

Nice, just small things to fix

bluss · 2018-06-13T17:17:37Z

    ///          [6, 6, 7, 7, 8, 8, 0]]));
    /// ```
-    pub fn exact_chunks_mut<E>(&mut self, chunk_size: E) -> ExactChunksMut<A, D> 
+    pub fn exact_chunks_mut<E>(&mut self, chunk_size: E) -> ExactChunksMut<A, D>


Please avoid formatting changes outside the code you are editing.

My vim settings removes trailing whitespaces on save... Got to change it when I am working on ndarray :P

I have Emacs set up the same way. Fwiw, I've found it useful to have the "save" keybinding set to remove trailing whitespace, but preserve the ability to call the save-buffer command directly if desired to save without removing whitespace.

Anyway, I've submitted PR #464 to remove all the trailing whitespace in ndarray so that we won't have to worry about this issue anymore.

For the purpose of this PR, though, @bluss is right that the PR should not reformat lines unrelated to the changes.

Yes, I was thinking about doing something similar to that in my .init.vim.
Until #464 lands I guess I'll be restoring whitespaces once the code has been approved and it is just waiting to be merged - final clean-up, so to speak.

bluss · 2018-06-13T17:20:40Z

+    {
+        let dim = self.dim.clone();
+        let strides = self.strides.clone();
+        if self.is_contiguous() {


This redundant is_contiguous call is unfortunate. The strides.clone call can move inside the first if branch though, so that's nice.

bluss · 2018-06-13T17:21:43Z

+        -> Array<B, D::Smaller>
+        where D: RemoveAxis,
+              F: FnMut(ArrayViewMut1<'a, A>) -> B,
+              A: 'a + Clone,


This A: Clone here is unwanted

The 'a here is good.

bluss · 2018-06-13T17:23:31Z

+    /// Return an array with the same shape as `self`.
+    pub fn map_mut<'a, B, F>(&'a mut self, f: F) -> Array<B, D>
+        where F: FnMut(&mut A) -> B,
+              A: 'a,


The 'a parameter now has no use and can be removed. (It having no use unlike the &self version of map, is correct.)

Hm.. wait a minute, that's not right, using 'a the same way as the &self map would be good.

bluss · 2018-06-13T17:26:08Z

+            unsafe {
+                mapping(ArrayViewMut::new_(first_elt, Ix1(view_len), Ix1(view_stride)))
+            }
+        })


This looks good!

LukeMathWalker · 2018-06-13T21:38:22Z

Thanks for reviewing @bluss!
I should have implemented/fixed all lines you pointed to (as well as restoring trailing whitespaces). :D

bluss · 2018-06-14T18:58:10Z

Thanks a lot for this!

LukeMathWalker added 3 commits May 29, 2018 07:28

Added to_vec_mapped_mut.

fa0c85c

Removed useless . Implemented TrustedIterator for IterMut and slice::…

3de28af

…IterMut

Implemented map_mut and map_axis_mut

85a83d1

LukeMathWalker mentioned this pull request Jun 2, 2018

Order statistics #461

Closed

bluss reviewed Jun 13, 2018

View reviewed changes

LukeMathWalker added 4 commits June 13, 2018 22:30

Moved strides clone call inside the happy if branch.

e4075bb

Emulating lifetime parameters usage in map.

b35a51b

Removing unneeded Clone bound on A in map_axis_mut.

153b1f2

Restoring whitespaces

2bf5cc9

bluss merged commit 7ac0600 into rust-ndarray:master Jun 14, 2018

LukeMathWalker mentioned this pull request Jun 15, 2018

map_axis with FnMut(ArrayViewMut1<'a, A>) #452

Closed

LukeMathWalker deleted the map_axis_mut branch June 15, 2018 06:59

Conversation

LukeMathWalker commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukeMathWalker Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukeMathWalker commented Jun 13, 2018

Uh oh!

bluss commented Jun 14, 2018

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LukeMathWalker commented May 29, 2018 •

edited

Loading

LukeMathWalker Jun 13, 2018 •

edited

Loading